Add hAdapter parameter to urPlatformCreateWithNativeHandle#1692
Add hAdapter parameter to urPlatformCreateWithNativeHandle#1692kbenzie merged 2 commits intooneapi-src:mainfrom
Conversation
5566671 to
f9638bc
Compare
|
I see that this patch makes sense. I'm not sure this really needs a cuda/hip review because I'm not sure that this interface is really ever going to be called by cuda/hip backend. For cuda and hip (and I guess some other backends), the interface of This is my perspective:
Therefore, IMO it should only be ever possible to construct a single platform in these backends, which includes all the devices that are available to the runtime. There was a proposal that we should define interop native platform in these backends as a vector of devices, which could imply that users could create multiple platforms from a cuda or hip backend, even if the runtime can only ever create one. Having multiple platforms in cuda/hip backends doesn't make any sense to me. We had to do it temporarily in cuda for awkward sycl::context spec issue but this has now been fixed. The fact that we previously had to have multiple platforms in the cuda backend might have been a motivation for this list of devices definition of the native platform, but I think that is out of date now. But @AerialMantis will know more about this I don't imagine that other runtimes would behave differently, but I don't know this for sure. If others agree then this might be a nice time to add some warning or comment in the cuda/hip adapter source stating this interface won't be called in cuda/hip for the above reasons. Just so in the future some poor soul doesn't get roped into trying to work out how to implement it for cuda/hip. |
|
Changes look fine to me, I'll approve this. I made some related points about the relevance of the usage of this function in cuda/hip. IMO we should add a comment in the cuda/hip implementations saying something to the effect that these interop functions don't make sense for cuda/hip backends. |
PietroGhg
left a comment
There was a problem hiding this comment.
Native CPU LGTM, thank you
f9638bc to
6da8a6c
Compare
|
added a comment explaining the unsupported error codes |
|
LLVM PR intel/llvm#14012 |
pbalcer
left a comment
There was a problem hiding this comment.
You can also modify the helper script to get rid of now redundant types in the loader:
#1067 (comment)
debd8e5 to
a44e5e6
Compare
|
@oneapi-src/level-zero-maintainers please review, this is important for the sycl UR port |
Other CreateWithNative entry points need a UR handle primarily so the loader has something to obtain a ddi table from. This new adapter parameter fulfils that purpose for PlatformCreateWithNative, allowing us to remove a weird hack that saw the loader getting (apparently succesfully somehow??) a loader object out of the native handle itself.
a44e5e6 to
fcecc00
Compare
pbalcer
left a comment
There was a problem hiding this comment.
The L0 adapter change is insignificant.
Other CreateWithNative entry points need a UR handle primarily so the loader has something to obtain a ddi table from. This new adapter parameter fulfils that purpose for PlatformCreateWithNative, allowing us to remove a weird hack that saw the loader getting (apparently succesfully somehow??) a loader object out of the native handle itself.
fixes #1068